-
Couldn't load subscription status.
- Fork 5.1k
Add ExtProc Logging Bits for Immediate Response, Continue and Replace #41602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add ExtProc Logging Bits for Immediate Response, Continue and Replace #41602
Conversation
…in FilterState Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
|
/assign @yanjunxiang-google |
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
|
/assign @tyxia |
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. Thanks for contribution!
I will let @yanjunxiang-google review it as well
| void onFinishProcessorCall(Grpc::Status::GrpcStatus call_status, | ||
| CallbackState next_state = CallbackState::Idle); | ||
| CallbackState next_state = CallbackState::Idle, | ||
| bool continue_and_replace = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: given this function is only called in two places, we can just specify continue_and_replace on the caller site, instead of having default arg
| // Test the ability of the filter to completely replace a request message with a new | ||
| // request message. | ||
| TEST_P(ExtProcIntegrationTest, ExtProcLoggingInfoContinueAndReplace) { | ||
| auto access_log_path = TestEnvironment::temporaryPath(TestUtility::uniqueFilename()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably should think about sharing the common code between tests. e.g., ExtProcLoggingInfoGRPCTimeout, to improve the code maintenance.
ext_proc_integration_test has grown significantly
Commit Message: Add bits to the FilterState in ExtProcLoggingInfo for when an immediate response is sent and when continue_and_replace is used.
Additional Description: These bits will be useful for internal debugging. Continue and Replace is only relevant for request and response headers.
Risk Level: Low
Testing: Unit Integration tests
Docs Changes: N/A
Release Notes: Add new bits into ExtProcLoggingInfo for Immediate Response occurrence and ContinueAndReplace.
Platform Specific Features: N/A
/assign @yanjunxiang-google